Skip to content

Conversation

@TheFifthRider
Copy link

@TheFifthRider TheFifthRider commented Oct 30, 2025

This should be marked as nullable :(

May resolve #106

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced robustness of block interaction mechanics to prevent potential crashes when certain interaction parameters are unavailable.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Walkthrough

The OnHeldInteractStop method in CollectibleBehaviorScrapeCrucible was modified to accept a nullable BlockSelection parameter. A null-check was added to prevent accessing .Position on a null blockSel, addressing a server-side NullReferenceException that caused player disconnections during chiseling.

Changes

Cohort / File(s) Summary
Null-safety fix for block selection
SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs
Method signature updated: OnHeldInteractStop parameter blockSel changed from BlockSelection to BlockSelection? (nullable). Early guard added in server logic to skip processing when blockSel or blockSel.Position is null, preventing NullReferenceException crashes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file with focused changes to one method
  • Straightforward null-safety pattern applied
  • Method signature change requires verification against base class or interface contracts
  • Ensure null-check logic doesn't skip critical server-side operations unintentionally

Poem

A chisel swung, a crash did strike, 🪨
Null references caused the blight,
Now guards stand tall at method's gate,
Block selections meet their fate,
No more disconnects in sight! 🐰✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Fixes NPE when spam clicking right click with a chisel" clearly and specifically describes the main change in the changeset. The code modifications directly address this objective by making the BlockSelection parameter nullable and adding null-safety checks to the OnHeldInteractStop method, which aligns with fixing the NullReferenceException reported in issue #106. The title is concise, non-vague, and accurately represents the primary purpose of the changes without unnecessary noise.
Linked Issues Check ✅ Passed The code changes directly address the requirements from issue #106. The PR makes the BlockSelection parameter nullable (BlockSelection?) and adds an early guard check that returns if blockSel.Position is null, which prevents the NullReferenceException that was causing server disconnects when chiseling. The implementation aligns with the implied objectives of properly annotating nullable values and adding necessary null-checks to prevent the crash on line 51 of CollectibleBehaviorScrapeCrucible.cs.
Out of Scope Changes Check ✅ Passed All changes in this pull request are directly in scope and related to fixing the NPE issue. Only one file is modified (CollectibleBehaviorScrapeCrucible.cs) with changes limited to the OnHeldInteractStop method signature and its null-safety implementation. The modifications—making BlockSelection nullable and adding null checks—are precisely what is needed to resolve the NullReferenceException reported in issue #106, with no extraneous or unrelated modifications present.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2)

30-37: Consider making blockSel nullable for consistency.

OnHeldInteractStep uses a non-nullable BlockSelection parameter while OnHeldInteractStart (line 13) and OnHeldInteractStop (line 42) both accept nullable BlockSelection?. For consistency and defensive coding, consider updating this signature to match the pattern:

 public override bool OnHeldInteractStep(float secondsUsed, ItemSlot slot, EntityAgent byEntity,
-    BlockSelection blockSel,
+    BlockSelection? blockSel,
     EntitySelection entitySel, ref EnumHandling handling)
 {
+    if (blockSel is null) return false;
     byEntity.StartAnimation("knifecut");
     handling = EnumHandling.PreventDefault;
     return secondsUsed < 1.5;
 }

130-135: Update method signature to accept nullable BlockSelection?.

After the fix, TryGetSelectedGroundStorage is called at line 52 with a BlockSelection? argument. To avoid potential compiler warnings and maintain consistency, update the method signature:

 private static BlockEntityGroundStorage? TryGetSelectedGroundStorage(EntityPlayer entityPlayer,
-    BlockSelection blockSel)
+    BlockSelection? blockSel)
 {
+    if (blockSel?.Position is null) return null;
     var blockEntity = entityPlayer.World.BlockAccessor.GetBlockEntity(blockSel.Position);
     return blockEntity as BlockEntityGroundStorage;
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef40e45 and 5fdc9a7.

📒 Files selected for processing (1)
  • SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2 hunks)
🔇 Additional comments (2)
SmithingPlus/BitsRecovery/CollectibleBehaviorScrapeCrucible.cs (2)

39-44: Good fix for the NPE issue.

Making blockSel nullable correctly aligns with the base class contract and matches the pattern already used in OnHeldInteractStart (line 13).


51-51: Null check correctly prevents the NPE.

The guard clause effectively addresses the server-side crash reported in issue #106 by ensuring both blockSel and its Position property are non-null before proceeding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disconnecting from server when chiseling

1 participant